-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added type check to webhook signature #10
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would of helped if I click the "submit review" button 🙄
This is unrelated to the PR, but I noticed we're doing
This is potentially problematic because of the following scenario
The problem is that the string in (1) and the string in (3) should be the same, but they're not guaranteed to be, and if they are not exactly the same, then the signature verification will fail. Getting the raw request body eliminates the risk of this. This is a common problem, and we document how to handle this in the node SDK: https://github.com/muxinc/mux-node-sdk#verifying-webhook-signatures and the elixir SDK: https://github.com/muxinc/mux-elixir#usage-in-phoenix |
Great question. What I can say is that Strapi's request apis are based on Koa.js (https://koajs.com/). It might be possible to capture the raw body and grab the string up front. That would have to be looked at. But I agree, it's possible that stringifying a json object might change the order of the properties. |
- Upgraded strapi peer dependency - Removed unneeded strapi-helper-plugin (it's global) - Upgraded strapi-utils dependency - Implemented proper strapi lang interpolation for strings - Introduced permissions scoping (currently, all admins can use)
Quick update, so the webhooks bit looks like it is addressed. However, and with more concern, it seems that something has changed with how Strapi handles plugins and exposing settings in the admin dashboard. What's happening is that "General" links in the Strapi dashboard will show initially allowing a user to go in and configure Strapi (including the Mux Video Uploader). However, after saving, the whole "General" section on the left menu of Strapi disappears. This includes Marketplace, Plugins and all of the settings (not just Mux Video Uploader). We've reached out to Strapi to see if we can get some understanding as to what is happening here. Will keep this thread apprised! |
- Establish plugin permissions for settings and main using bootstrap - Updated models and services to use new model schemes
- plugins are formatted to remove the `strapi-plugin-` for npm installs
- Commented out webhook signature check (see README.md) - Updated README.md with notes on webhook caveat - Update README.md with more accurate permission explanation
With Strapi's help, we were able to resolve the permission issues and it's working 🎉 . In summary, here is what we've done:
@tbobkermux could you run through some testing before we merge this in? |
- Added steps for publishing release
I've implemented a conditional check to see if the signature header is a string or if it is an array and removed the error to upgrade axios package to the latest version.